change the type of the argument of drop_in_place lang item to &mut _#154327
change the type of the argument of drop_in_place lang item to &mut _#154327rust-bors[bot] merged 8 commits intorust-lang:mainfrom
drop_in_place lang item to &mut _#154327Conversation
|
Oh wow I hadn't expected my wish to be fulfilled before I could even finish my PR that motivated me to express the wish in the first place. :-) ❤️ |
This comment has been minimized.
This comment has been minimized.
281786e to
4842194
Compare
This comment has been minimized.
This comment has been minimized.
794fcf5 to
ea0c913
Compare
This comment has been minimized.
This comment has been minimized.
ea0c913 to
3dda404
Compare
This comment has been minimized.
This comment has been minimized.
3dda404 to
32510c2
Compare
This comment has been minimized.
This comment has been minimized.
|
I like the interpreter changes and renames. Not sure what the status of the rest of the PR is, but if you submit just those as a separate PR I'll r+. |
32510c2 to
487fcc4
Compare
This comment has been minimized.
This comment has been minimized.
487fcc4 to
6e43096
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…s, r=RalfJung Slightly refactor mplace<->ptr conversions split off of rust-lang#154327 r? RalfJung
e35368d to
94e4ddf
Compare
|
@bors try jobs=test-various |
This comment has been minimized.
This comment has been minimized.
change the type of the argument of `drop_in_place` lang item to `&mut _` try-job: test-various
|
@bors r=RalfJung,scottmcm,saethlin |
This comment has been minimized.
This comment has been minimized.
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
☀️ Test successful - CI, Post merge analysis |
3 similar comments
|
☀️ Test successful - CI, Post merge analysis |
|
☀️ Test successful - CI, Post merge analysis |
|
☀️ Test successful - CI, Post merge analysis |
|
Finished benchmarking commit (4ddd453): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.2%, secondary 3.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 7.3%, secondary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 500.816s -> 499.861s (-0.19%) |
|
☀️ Test successful - CI, Post merge analysis |
5 similar comments
|
☀️ Test successful - CI, Post merge analysis |
|
☀️ Test successful - CI, Post merge analysis |
|
☀️ Test successful - CI, Post merge analysis |
|
☀️ Test successful - CI, Post merge analysis |
|
☀️ Test successful - CI, Post merge analysis |
|
@Kobzol uhhhhh... it's bors supposed to do this? |
View all comments
We used to special case
core::ptr::drop_in_placewhen computing LLVM argument attributes with this hack:rust/compiler/rustc_ty_utils/src/abi.rs
Lines 383 to 392 in db5e2dc
This is because even though
drop_in_placetakes a*mut Tit is semantically a&mut T(remember how&mut Selfis passed toDrop::drop). This is apparently relevant for perf.This PR replaces this hack with a simpler solution -- it makes
drop_in_placea thin wrapper around newly addedcore::ptr::drop_glue, which is the actual lang item and takes a&mut T:rust/library/core/src/ptr/mod.rs
Lines 810 to 833 in d2563d5
The rest of the PR is blessing tests and cleaning up things which are not necessary after this change.
One thing that is a bit awkward is that now that
drop_glueis the actual lang item, a lot of the comments referring todrop_in_placeare outdated. Should I try fixing that?I've also changed
async_drop_in_placeto take a&mut T, and it simplified the code handling it a bit. (since it's unstable we don't need to introduce a wrapper)cc @RalfJung
Closes #154274